fix: enhance count argument handling in WebSearchTool to support string input#741
fix: enhance count argument handling in WebSearchTool to support string input#741Xeven777 wants to merge 1 commit intosipeed:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request modifies the WebSearchTool.Execute method to handle the count parameter more flexibly by accepting both float64 (from JSON) and string inputs. The changes use a switch statement with type assertions to parse and validate the count parameter, adding string-to-integer conversion via strconv.Atoi and strict integer validation for float64 values using math.Round and math.Abs.
Changes:
- Replaced simple float64 type assertion with a switch statement handling both float64 and string types
- Added strict validation to reject fractional float64 values
- Added string parsing support using strconv.Atoi
- Imported math and strconv packages
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch v := args["count"].(type) { | ||
| case float64: | ||
| rounded := int(math.Round(v)) | ||
| if math.Abs(v-float64(rounded)) > 1e-9 { | ||
| return ErrorResult(fmt.Sprintf("count must be an integer, got %v", v)) | ||
| } | ||
| if rounded > 0 && rounded <= 10 { | ||
| count = rounded | ||
| } | ||
| case string: | ||
| if n, err := strconv.Atoi(v); err == nil && n > 0 && n <= 10 { | ||
| count = n | ||
| } | ||
| } |
There was a problem hiding this comment.
The new count parameter handling logic (both float64 and string cases) lacks test coverage. The existing test file web_test.go has no tests for the count parameter. Consider adding tests for: valid integer values (as float64), fractional values that should be rejected, string inputs (valid and invalid), boundary values (0, 1, 10, 11), and the default fallback behavior when count is omitted or invalid.
| rounded := int(math.Round(v)) | ||
| if math.Abs(v-float64(rounded)) > 1e-9 { | ||
| return ErrorResult(fmt.Sprintf("count must be an integer, got %v", v)) | ||
| } | ||
| if rounded > 0 && rounded <= 10 { | ||
| count = rounded | ||
| } |
There was a problem hiding this comment.
The new validation that rejects fractional float64 values (lines 481-482) is a breaking change from the previous behavior, which silently converted fractional values to integers. If the count parameter receives 5.5, the old code would have converted it to 5 and used it, but the new code returns an error. Consider whether this breaking change is intentional and justified, or if you should maintain backward compatibility by silently truncating/rounding fractional values as the original code did.
| rounded := int(math.Round(v)) | |
| if math.Abs(v-float64(rounded)) > 1e-9 { | |
| return ErrorResult(fmt.Sprintf("count must be an integer, got %v", v)) | |
| } | |
| if rounded > 0 && rounded <= 10 { | |
| count = rounded | |
| } | |
| n := int(v) | |
| if n > 0 && n <= 10 { | |
| count = n | |
| } |
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "math" |
There was a problem hiding this comment.
The math import is only used for the strict integer validation (math.Round and math.Abs). If the fractional value validation is removed (as suggested in other comments), this import would no longer be needed and should be removed to avoid unnecessary dependencies.
| "math" |
| case string: | ||
| if n, err := strconv.Atoi(v); err == nil && n > 0 && n <= 10 { | ||
| count = n | ||
| } |
There was a problem hiding this comment.
Adding string support for the count parameter deviates from the established pattern in the codebase. Other tools with integer parameters (like skills_search.go line 62, cron.go lines 149-150, i2c_linux.go line 159) only accept float64, not strings. The Parameters() method declares count as type "integer", which means LLM APIs will send it as a number (decoded as float64 in Go). Unless there's a specific use case requiring string support (e.g., manual testing, specific provider quirks), consider removing the string case to maintain consistency with other tools in the codebase.
| case string: | |
| if n, err := strconv.Atoi(v); err == nil && n > 0 && n <= 10 { | |
| count = n | |
| } |
| rounded := int(math.Round(v)) | ||
| if math.Abs(v-float64(rounded)) > 1e-9 { | ||
| return ErrorResult(fmt.Sprintf("count must be an integer, got %v", v)) | ||
| } | ||
| if rounded > 0 && rounded <= 10 { | ||
| count = rounded | ||
| } |
There was a problem hiding this comment.
The strict integer validation for float64 values is inconsistent with how other tools in the codebase handle numeric parameters. For example, skills_search.go (line 62-63) and cron.go (line 155, 161) simply cast float64 to int without checking for fractional parts. When JSON is unmarshaled into map[string]any, integer values become float64 by default in Go, so values like 5.0 are expected and valid. The current implementation would reject 5.0000000001 due to floating-point precision issues, which could cause unexpected failures. Consider either removing this validation to match the codebase pattern or at least using int(v) instead of math.Round if you want to be lenient with very small fractional parts.
| rounded := int(math.Round(v)) | |
| if math.Abs(v-float64(rounded)) > 1e-9 { | |
| return ErrorResult(fmt.Sprintf("count must be an integer, got %v", v)) | |
| } | |
| if rounded > 0 && rounded <= 10 { | |
| count = rounded | |
| } | |
| n := int(v) | |
| if n > 0 && n <= 10 { | |
| count = n | |
| } |
| count = rounded | ||
| } | ||
| case string: | ||
| if n, err := strconv.Atoi(v); err == nil && n > 0 && n <= 10 { |
There was a problem hiding this comment.
When the string parsing fails or the value is out of range, the code silently falls back to the default maxResults without informing the user. This is inconsistent with the float64 case (lines 481-482) which returns an error for invalid values. Consider either returning an error for invalid string inputs to maintain consistency, or document why strings are treated more leniently than floats. The silent fallback could confuse users who expect their count parameter to be respected.
| if n, err := strconv.Atoi(v); err == nil && n > 0 && n <= 10 { | |
| n, err := strconv.Atoi(v) | |
| if err != nil { | |
| return ErrorResult(fmt.Sprintf("count must be an integer, got %q", v)) | |
| } | |
| if n > 0 && n <= 10 { |
Xeven777
force-pushed
the
fix/web-count-type-handling
branch
from
4809885 to
52a137f
Compare
|
@xiaket review ? |
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Clean fix for the type-mismatch issue where LLMs send count as a string instead of a number.
Two small things:
-
The float64 non-integer check feels over-engineered for this use case. The original code just did
int(c)which truncates. Yourmath.Round+ epsilon comparison returns an error forcount: 3.5, but silently acceptscount: 15(out of range, falls through to the default). The asymmetry is a bit odd -- if you are going to validate, it would be more consistent to also return an error for out-of-range values rather than silently using the default. -
Missing test coverage. This is a behavioral change (string and int types now accepted, non-integer floats rejected). Even a small table-driven test would prevent regressions and document the expected behavior for each type.
This pull request updates the handling of the
countargument in theWebSearchTool.Executemethod to make it more robust. The main improvement is that the method can now accept both numeric and string values for thecountparameter, converting strings to integers when possible.Improvements to argument handling:
countargument inWebSearchTool.Executeto accept bothfloat64and string types, converting string values to integers usingstrconv.Atoiand validating the range (1–10).strconvpackage import inpkg/tools/web.goto support string-to-integer conversion.